Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat common: slight code improvement #9

Closed
wants to merge 3 commits into from
Closed

feat common: slight code improvement #9

wants to merge 3 commits into from

Conversation

algol83
Copy link

@algol83 algol83 commented Apr 12, 2024

No description provided.

algol83
algol83 commented Apr 12, 2024
View reviewed changes
tests/test_utils.h Outdated Show resolved Hide resolved
d-tatianin
d-tatianin reviewed Apr 12, 2024
View reviewed changes
CMakeLists.txt
@@ -17,6 +17,7 @@ message("Libvhost log verbosity: ${LIBVHOST_LOG_VERBOSITY}")
add_library(vhost-server)
add_compile_definitions(_GNU_SOURCE LOG_VERBOSITY=${LIBVHOST_LOG_VERBOSITY})
target_compile_options(vhost-server PRIVATE
-std=gnu17
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CMakeLists.txt was introduced by NBS since they integrate libvhost as part of their project, we don't actually build with it locally.
This will need to be checked in with them (might break their build).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code is compiled exclusively by gnu extensions of the standard, it is worth explicitly denoting this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aikuchin Would you be okay with this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need 17 here? I was quite sure that 11 was enough, maybe 14, but 17 seems like an overkill and will limit supported compilers significantly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C14 does not exist. C17 is supported by GCC 8.1 and Clang 7. C23 causes confusion.

Copy link
Contributor

@aikuchin aikuchin Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to understand the motivation for gnu17 if we only use features for gnu11. Why do we need 17, do we ?
If we need new compiler features it is easy to bump requirements in new version, but downgrading standard is always more complicated.
UPD: Since c17 is just a bugfix release for c11 I agree that gnu17 is OK here.

And should we also add required standard version to meson.build?

d-tatianin
d-tatianin reviewed Apr 12, 2024
View reviewed changes
CMakeLists.txt
@@ -61,3 +62,6 @@ target_sources(vhost-server PRIVATE
virtio/virtio_blk.c
virtio/virtio_fs.c
)


add_subdirectory(tests)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I'm not sure NBS needs the tests subdirectory for their integration (and this might break their isolated build as well)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very unusual to be selective about build systems and focus on only one consumer for an open source project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building tests makes sense and NBS will fix their build if building tests break it. So I think this is a good idea to build tests.

d-tatianin
d-tatianin reviewed Apr 15, 2024
View reviewed changes
vdev.c Show resolved Hide resolved
@d-tatianin
Copy link
Collaborator

Can you please clean up the history to make commits atomic and have a description of why/how they improve the code?
Generally we try to avoid bulk changes in a single commit as it makes it impossible to bisect or revert changes.

Ideally i'd want something like this:

Commit 1:

everywhere: use angled include where applicable

This is better because ...

Signed-off-by: ...

Commit 2:

test_utils: use localtime_r over localtime

It's better to use localtime_r because...

Signed-off-by: ...

Commit N...

aikuchin
aikuchin reviewed Apr 18, 2024
View reviewed changes
bio.h Outdated Show resolved Hide resolved
@algol83 algol83 force-pushed the feature/small-upgrade branch 2 times, most recently from 01559c4 to aed2710 Compare May 13, 2024 12:40
aikuchin
aikuchin suggested changes May 14, 2024
View reviewed changes
bio.h Outdated Show resolved Hide resolved
aikuchin
aikuchin approved these changes Jun 14, 2024
View reviewed changes
Copy link
Contributor

@aikuchin aikuchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I have some NITs but no strong objections.

tests/test_utils.h Outdated Show resolved Hide resolved
CMakeLists.txt
@@ -17,6 +17,7 @@ message("Libvhost log verbosity: ${LIBVHOST_LOG_VERBOSITY}")
add_library(vhost-server)
add_compile_definitions(_GNU_SOURCE LOG_VERBOSITY=${LIBVHOST_LOG_VERBOSITY})
target_compile_options(vhost-server PRIVATE
-std=gnu17
Copy link
Contributor

@aikuchin aikuchin Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to understand the motivation for gnu17 if we only use features for gnu11. Why do we need 17, do we ?
If we need new compiler features it is easy to bump requirements in new version, but downgrading standard is always more complicated.
UPD: Since c17 is just a bugfix release for c11 I agree that gnu17 is OK here.

And should we also add required standard version to meson.build?

CMakeLists.txt
@@ -61,3 +62,6 @@ target_sources(vhost-server PRIVATE
virtio/virtio_blk.c
virtio/virtio_fs.c
)


add_subdirectory(tests)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building tests makes sense and NBS will fix their build if building tests break it. So I think this is a good idea to build tests.

Aleksandr Golubev added 3 commits June 17, 2024 17:41
cmake: build tests
f30b414 
To check when modifying code when using сmake, it is worth collecting all binary files.
The code is compiled only with gnu extensions of the standard. This should be explicitly stated.

Signed-off-by: Aleksandr Golubev <[email protected]>
tests: use a thread-safe function to transform time
1cc984a 
The test logging contains a call to a not-thread-save method localtime.

Signed-off-by: Aleksandr Golubev <[email protected]>
build: fixing unused-result warning
2cee9b2 
The results of calls to methods write() and read() are now checked in the debug build, which will help diagnose potential problems.

Signed-off-by: Aleksandr Golubev <[email protected]>
@algol83 algol83 closed this Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-tatianin d-tatianin d-tatianin left review comments

@aikuchin aikuchin aikuchin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@algol83 @d-tatianin @aikuchin